Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Convert to using list of accessibility elements for iOS tab indexes #11077

Merged
merged 4 commits into from
Jul 16, 2020
Merged

Convert to using list of accessibility elements for iOS tab indexes #11077

merged 4 commits into from
Jul 16, 2020

Conversation

PureWeen
Copy link
Contributor

@PureWeen PureWeen commented Jun 16, 2020 •
edited
Loading

Description of Change

Convert to using list of accessibility elements over methods. My theory here is that the performance of the individual methods when going across the bridge was causing hiccups with Appium.

I noticed on a ListView that it would call the "getMethods" for every single elements as you moved the mouse around which caused it to move incredibly slow.

Once I swapped over to just using the getter it all seemed to work much smoother

#9702 (comment)

Issues Resolved

Platforms Affected

  • iOS

Testing Procedure

PR Checklist

  • Targets the correct branch
  • Tests are passing (or failures are unrelated)

@PureWeen PureWeen added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Jun 16, 2020
@PureWeen PureWeen requested a review from samhouts June 16, 2020 18:17
@samhouts samhouts added a/webview i/regression t/bug 🐛 4.0.0 regression on 4.0.0 i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often m/high impact ⬛ partner/cat 😻 labels Jun 16, 2020
PureWeen
PureWeen commented Jun 17, 2020
View reviewed changes
Xamarin.Forms.Platform.iOS/Renderers/PageContainer.cs
// lazy-loading this list so that the expensive call to GetAccessibilityElements only happens when VoiceOver is on.
if (_accessibilityElements == null || _accessibilityElements.Count == 0)
{
_accessibilityElements = _parent.GetAccessibilityElements();
var elements =_parent.GetAccessibilityElements();
if(elements != null)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation for this property says the default is null
https://developer.apple.com/documentation/objectivec/nsobject/1615147-accessibilityelements

Containers can implement this property instead of the dynamic methods to support the retrieval of the contained elements. The default value of this property is nil.

When I do that it seems like the accessibility just returns to the default behavior. I added all the repro's from the PR listed here #9702 and I tested the Accessibility demo on xamarin-samples.

We might need to add the setAccessibilityElements side of this as well for certain edge cases.

For now we'll keep it simple and see if that works

@samhouts samhouts added the Core label Jun 17, 2020
@PureWeen
Copy link
Contributor Author

Failures unrelated

@PureWeen
Copy link
Contributor Author

@arevellfraedom summarized these properties really well here #9702 (comment)

@arevellfraedom I've tested through all the issues associated with issue #9702 and added UI Tests for them as well

If you have any additional cases or things you feel this PR might break please let me know

AFAICT this statement you made

I think sending null might even remove an override, but haven't checked.

Is true

Here's the nuget if you want to test
https://dev.azure.com/xamarin/public/_build/results?buildId=21211&view=artifacts&type=publishedArtifacts

@PureWeen
Copy link
Contributor Author

All green

@samhouts samhouts changed the base branch from 4.7.0 to master June 25, 2020 22:38
@samhouts samhouts changed the base branch from master to 4.8.0 June 26, 2020 00:00
@PureWeen PureWeen mentioned this pull request Jul 10, 2020
Closed
@PureWeen PureWeen removed the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Jul 15, 2020
samhouts
samhouts approved these changes Jul 16, 2020
View reviewed changes
Copy link
Member

@samhouts samhouts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wunderbar!

@samhouts samhouts added the blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. label Jul 16, 2020
@samhouts samhouts merged commit 35e5dc9 into 4.8.0 Jul 16, 2020
@samhouts samhouts deleted the ios_appium_fixes branch July 16, 2020 23:20
@samhouts samhouts modified the milestone: 4.8.0 Jul 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@samhouts samhouts samhouts approved these changes

@jsuarezruiz jsuarezruiz Awaiting requested review from jsuarezruiz

Assignees

@jsuarezruiz jsuarezruiz

@samhouts samhouts

Labels
4.0.0 regression on 4.0.0 a/a11y 🔍 a/webview blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. Core i/high Completely doesn't work, crashes, or is unusably slow, has no obvious workaround; occurs less often i/regression m/high impact ⬛ p/iOS 🍎 partner/cat 😻 t/bug 🐛
Projects
None yet
Milestone
4.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@PureWeen @samhouts @jsuarezruiz